P1B: Refactor (packages/opencode/src/cli/error.ts:35): Function with high complexity (count = 37): FormatError - #75
Open
haq2026 wants to merge 4 commits into
Conversation
Author
|
CI update: the I verified by checking out the upstream
The failing test is I also pushed a small follow-up commit restoring |
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.





1. Issue
Link to the associated GitHub issue:
Closes #74
Full path to the refactored file:
packages/opencode/src/cli/error.tsWhat do you think this file does?
It converts internal error objects into human-readable messages for the CLI.
FormatErrorrecognizes known error types (config errors, provider errors, account errors, CLI errors, UI cancellation) and returns a formatted string for each;FormatUnknownErroris the fallback for anything unrecognized.What is the scope of your refactoring within that file?
Only
FormatErrorand the new helper functions extracted from it.FormatUnknownErrorand the existing private helpers (isTaggedError,configData,stringField,configIssues) are unchanged.2. Code smell
Which code smell did you fix?
Qlty flagged
FormatErrorat line 35 for two smells: high complexity (count = 37) and many returns (count = 13).Qlty output before:
Qlty output after: no smells reported for this file.
What did you change?
Each error-type branch was extracted into its own named formatter function (
formatCliError,formatMcpFailed,formatAccountError,formatProviderModelNotFound,formatProviderInit,formatConfigJson,formatConfigDirectoryTypo,formatConfigFrontmatter,formatConfigRemoteAuth,formatConfigInvalid,formatUICancelled).FormatErrornow iterates an orderedFORMATTERSarray and returns the first non-undefined result. The array preserves the original branch order, so precedence is unchanged, and theprocess.exitCodeside effect stays inside the CliError branch exactly as before.3. Testing
How did you verify your change?
packages/opencode/test/cli/error.test.tswas extended from 6 tests to 13. The existing 6 already covered the config, provider model, provider init and UI cancellation branches; the 7 new tests cover the previously untested paths: CliError with and without a message (including assertingprocess.exitCodeis set to the error's exit code), remote config auth errors with and without a URL, config invalid with no path and no issues, thecause.bodyunwrapping path, and the fallback returningundefinedfor unrecognized input.Which tests cover the refactored code, and why are they sufficient?
All 13 tests in
packages/opencode/test/cli/error.test.tsexerciseFormatErrordirectly. Line coverage onsrc/cli/error.tsis 106 of 108 lines (98.1%); the only two uncovered lines are the MCPFailed branch, which was untested before this change as well and is unmodified by it. Because this refactor is strictly behavior-preserving, every branch that changed shape is asserted against the exact same output string it produced before, so any drift in message text, branch ordering, or the exit-code side effect would fail these tests. They pass.Test results:
bun test test/cli/error.test.tsgives 13 pass, 0 failchunkTimeout raises a response stream error when SSE body stalls, a network-timeout test that was already failing onmainbefore this branch and is unrelated to this change.bun lintgives 698 warnings and 2 errors, identical to the pre-existing count onmain. No new lint issues introduced.Note on the diff: this branch also contains the
.devcontainer/devcontainer.jsoncommit from P1A, which lives on my fork'smainand has not been merged upstream. It is unrelated to this refactor.